From bdd477ad345e8e0ba6640ea480fa47a7c3d5fdf0 Mon Sep 17 00:00:00 2001 From: "emellor@ewan" Date: Fri, 23 Sep 2005 14:28:16 +0100 Subject: [PATCH] Add check for speed (takes 33 minutes on my laptop, OUCH!) Make xenstored use tdb, transactions can soft-fail (EAGAIN) Transactions no longer take root dir, no longer lock & block: commit can fail spuriously with EAGAIN, not ETIMEDOUT. Speeds up transactions by over 1000 times, should be NFS safe. New program: xs_tdb_dump to dump raw TDB contents. Don't do failure testing: we are no longer robust against all ENOMEM 8( Introduce "struct node" which contains perms, children and data. Make struct xs_permissions unpadded, so we can write to tdb w/o valgrind complaints. Gently modify TDB to use talloc, not do alloc on tdb_delete. Fix up transaction users for new semantics. Don't need a transaction around a single read in xen/i386/kernel/smpboot.c. Python: transaction_start() returns True/False rather than raising exception on EAGAIN. Fix usage comment on xs_transaction_end(). Include stdarg to xs_tdb_dump so it compiles. Signed-off-by: Rusty Russell --- .../arch/xen/i386/kernel/smpboot.c | 2 - linux-2.6-xen-sparse/arch/xen/kernel/reboot.c | 8 +-- .../drivers/xen/blkback/xenbus.c | 13 +++- .../drivers/xen/blkfront/blkfront.c | 5 +- .../drivers/xen/netfront/netfront.c | 5 +- .../drivers/xen/xenbus/xenbus_xs.c | 5 +- linux-2.6-xen-sparse/include/asm-xen/xenbus.h | 2 +- tools/python/xen/lowlevel/xs/xs.c | 20 +++--- tools/python/xen/xend/XendDomainInfo.py | 28 ++++---- tools/python/xen/xend/server/DevController.py | 29 ++++---- tools/python/xen/xend/xenstore/xsnode.py | 4 +- tools/python/xen/xend/xenstore/xstransact.py | 70 ++++--------------- tools/xenstore/xenstore_client.c | 11 +-- tools/xenstore/xs.h | 4 +- tools/xenstore/xs_tdb_dump.c | 1 + 15 files changed, 92 insertions(+), 115 deletions(-) diff --git a/linux-2.6-xen-sparse/arch/xen/i386/kernel/smpboot.c b/linux-2.6-xen-sparse/arch/xen/i386/kernel/smpboot.c index aaf20c2fce..cb6bfd6024 100644 --- a/linux-2.6-xen-sparse/arch/xen/i386/kernel/smpboot.c +++ b/linux-2.6-xen-sparse/arch/xen/i386/kernel/smpboot.c @@ -1394,9 +1394,7 @@ static void handle_vcpu_hotplug_event(struct xenbus_watch *watch, const char *no return; /* get the state value */ - xenbus_transaction_start("cpu"); err = xenbus_scanf(dir, "availability", "%s", state); - xenbus_transaction_end(0); if (err != 1) { printk(KERN_ERR diff --git a/linux-2.6-xen-sparse/arch/xen/kernel/reboot.c b/linux-2.6-xen-sparse/arch/xen/kernel/reboot.c index 5983e64c64..ba5bb57b58 100644 --- a/linux-2.6-xen-sparse/arch/xen/kernel/reboot.c +++ b/linux-2.6-xen-sparse/arch/xen/kernel/reboot.c @@ -324,7 +324,7 @@ static void shutdown_handler(struct xenbus_watch *watch, const char *node) int err; again: - err = xenbus_transaction_start("control"); + err = xenbus_transaction_start(); if (err) return; str = (char *)xenbus_read("control", "shutdown", NULL); @@ -337,7 +337,7 @@ static void shutdown_handler(struct xenbus_watch *watch, const char *node) xenbus_write("control", "shutdown", ""); err = xenbus_transaction_end(0); - if (err == -ETIMEDOUT) { + if (err == -EAGAIN) { kfree(str); goto again; } @@ -366,7 +366,7 @@ static void sysrq_handler(struct xenbus_watch *watch, const char *node) int err; again: - err = xenbus_transaction_start("control"); + err = xenbus_transaction_start(); if (err) return; if (!xenbus_scanf("control", "sysrq", "%c", &sysrq_key)) { @@ -379,7 +379,7 @@ static void sysrq_handler(struct xenbus_watch *watch, const char *node) xenbus_printf("control", "sysrq", "%c", '\0'); err = xenbus_transaction_end(0); - if (err == -ETIMEDOUT) + if (err == -EAGAIN) goto again; if (sysrq_key != '\0') { diff --git a/linux-2.6-xen-sparse/drivers/xen/blkback/xenbus.c b/linux-2.6-xen-sparse/drivers/xen/blkback/xenbus.c index c3296436e3..aaea46f231 100644 --- a/linux-2.6-xen-sparse/drivers/xen/blkback/xenbus.c +++ b/linux-2.6-xen-sparse/drivers/xen/blkback/xenbus.c @@ -80,8 +80,9 @@ static void frontend_changed(struct xenbus_watch *watch, const char *node) return; } +again: /* Supply the information about the device the frontend needs */ - err = xenbus_transaction_start(be->dev->nodename); + err = xenbus_transaction_start(); if (err) { xenbus_dev_error(be->dev, err, "starting transaction"); return; @@ -119,7 +120,15 @@ static void frontend_changed(struct xenbus_watch *watch, const char *node) goto abort; } - xenbus_transaction_end(0); + err = xenbus_transaction_end(0); + if (err == EAGAIN) + goto again; + if (err) { + xenbus_dev_error(be->dev, err, "ending transaction", + ring_ref, evtchn); + goto abort; + } + xenbus_dev_ok(be->dev); return; diff --git a/linux-2.6-xen-sparse/drivers/xen/blkfront/blkfront.c b/linux-2.6-xen-sparse/drivers/xen/blkfront/blkfront.c index 16f7657400..90c20b072f 100644 --- a/linux-2.6-xen-sparse/drivers/xen/blkfront/blkfront.c +++ b/linux-2.6-xen-sparse/drivers/xen/blkfront/blkfront.c @@ -572,7 +572,8 @@ static int talk_to_backend(struct xenbus_device *dev, goto out; } - err = xenbus_transaction_start(dev->nodename); +again: + err = xenbus_transaction_start(); if (err) { xenbus_dev_error(dev, err, "starting transaction"); goto destroy_blkring; @@ -603,6 +604,8 @@ static int talk_to_backend(struct xenbus_device *dev, err = xenbus_transaction_end(0); if (err) { + if (err == EAGAIN) + goto again; xenbus_dev_error(dev, err, "completing transaction"); goto destroy_blkring; } diff --git a/linux-2.6-xen-sparse/drivers/xen/netfront/netfront.c b/linux-2.6-xen-sparse/drivers/xen/netfront/netfront.c index fe496ef585..39e96846af 100644 --- a/linux-2.6-xen-sparse/drivers/xen/netfront/netfront.c +++ b/linux-2.6-xen-sparse/drivers/xen/netfront/netfront.c @@ -1122,7 +1122,8 @@ static int talk_to_backend(struct xenbus_device *dev, goto out; } - err = xenbus_transaction_start(dev->nodename); +again: + err = xenbus_transaction_start(); if (err) { xenbus_dev_error(dev, err, "starting transaction"); goto destroy_ring; @@ -1160,6 +1161,8 @@ static int talk_to_backend(struct xenbus_device *dev, err = xenbus_transaction_end(0); if (err) { + if (err == EAGAIN) + goto again; xenbus_dev_error(dev, err, "completing transaction"); goto destroy_ring; } diff --git a/linux-2.6-xen-sparse/drivers/xen/xenbus/xenbus_xs.c b/linux-2.6-xen-sparse/drivers/xen/xenbus/xenbus_xs.c index 948da0a3a4..50919961f4 100644 --- a/linux-2.6-xen-sparse/drivers/xen/xenbus/xenbus_xs.c +++ b/linux-2.6-xen-sparse/drivers/xen/xenbus/xenbus_xs.c @@ -287,12 +287,11 @@ EXPORT_SYMBOL(xenbus_rm); /* Start a transaction: changes by others will not be seen during this * transaction, and changes will not be visible to others until end. - * Transaction only applies to the given subtree. * You can only have one transaction at any time. */ -int xenbus_transaction_start(const char *subtree) +int xenbus_transaction_start(void) { - return xs_error(xs_single(XS_TRANSACTION_START, subtree, NULL)); + return xs_error(xs_single(XS_TRANSACTION_START, "", NULL)); } EXPORT_SYMBOL(xenbus_transaction_start); diff --git a/linux-2.6-xen-sparse/include/asm-xen/xenbus.h b/linux-2.6-xen-sparse/include/asm-xen/xenbus.h index 03d644b8d8..8b2e9482e3 100644 --- a/linux-2.6-xen-sparse/include/asm-xen/xenbus.h +++ b/linux-2.6-xen-sparse/include/asm-xen/xenbus.h @@ -87,7 +87,7 @@ int xenbus_write(const char *dir, const char *node, const char *string); int xenbus_mkdir(const char *dir, const char *node); int xenbus_exists(const char *dir, const char *node); int xenbus_rm(const char *dir, const char *node); -int xenbus_transaction_start(const char *subtree); +int xenbus_transaction_start(void); int xenbus_transaction_end(int abort); /* Single read and scanf: returns -errno or num scanned if > 0. */ diff --git a/tools/python/xen/lowlevel/xs/xs.c b/tools/python/xen/lowlevel/xs/xs.c index c0d1e9d475..186455f6f6 100644 --- a/tools/python/xen/lowlevel/xs/xs.c +++ b/tools/python/xen/lowlevel/xs/xs.c @@ -582,9 +582,8 @@ static PyObject *xspy_unwatch(PyObject *self, PyObject *args, PyObject *kwds) } #define xspy_transaction_start_doc "\n" \ - "Start a transaction on a path.\n" \ + "Start a transaction.\n" \ "Only one transaction can be active at a time.\n" \ - " path [string]: xenstore path.\n" \ "\n" \ "Returns None on success.\n" \ "Raises RuntimeError on error.\n" \ @@ -593,8 +592,8 @@ static PyObject *xspy_unwatch(PyObject *self, PyObject *args, PyObject *kwds) static PyObject *xspy_transaction_start(PyObject *self, PyObject *args, PyObject *kwds) { - static char *kwd_spec[] = { "path", NULL }; - static char *arg_spec = "s|"; + static char *kwd_spec[] = { NULL }; + static char *arg_spec = ""; char *path = NULL; struct xs_handle *xh = xshandle(self); @@ -606,7 +605,7 @@ static PyObject *xspy_transaction_start(PyObject *self, PyObject *args, if (!PyArg_ParseTupleAndKeywords(args, kwds, arg_spec, kwd_spec, &path)) goto exit; Py_BEGIN_ALLOW_THREADS - xsval = xs_transaction_start(xh, path); + xsval = xs_transaction_start(xh); Py_END_ALLOW_THREADS if (!xsval) { PyErr_SetFromErrno(PyExc_RuntimeError); @@ -623,7 +622,7 @@ static PyObject *xspy_transaction_start(PyObject *self, PyObject *args, "Attempts to commit the transaction unless abort is true.\n" \ " abort [int]: abort flag (default 0).\n" \ "\n" \ - "Returns None on success.\n" \ + "Returns True on success, False if you need to try again.\n" \ "Raises RuntimeError on error.\n" \ "\n" @@ -646,11 +645,16 @@ static PyObject *xspy_transaction_end(PyObject *self, PyObject *args, xsval = xs_transaction_end(xh, abort); Py_END_ALLOW_THREADS if (!xsval) { + if (errno == EAGAIN) { + Py_INCREF(Py_False); + val = Py_False; + goto exit; + } PyErr_SetFromErrno(PyExc_RuntimeError); goto exit; } - Py_INCREF(Py_None); - val = Py_None; + Py_INCREF(Py_True); + val = Py_True; exit: return val; } diff --git a/tools/python/xen/xend/XendDomainInfo.py b/tools/python/xen/xend/XendDomainInfo.py index 00ab50a53a..8180eda9ac 100644 --- a/tools/python/xen/xend/XendDomainInfo.py +++ b/tools/python/xen/xend/XendDomainInfo.py @@ -839,20 +839,20 @@ class XendDomainInfo: """Release all vm devices. """ - t = xstransact("%s/device" % self.path) - - for n in controllerClasses.keys(): - for d in t.list(n): - try: - t.remove(d) - except ex: - # Log and swallow any exceptions in removal -- there's - # nothing more we can do. - log.exception( - "Device release failed: %s; %s; %s; %s" % - (self.info['name'], n, d, str(ex))) - t.commit() - + while True: + t = xstransact("%s/device" % self.path) + for n in controllerClasses.keys(): + for d in t.list(n): + try: + t.remove(d) + except ex: + # Log and swallow any exceptions in removal -- + # there's nothing more we can do. + log.exception( + "Device release failed: %s; %s; %s; %s" % + (self.info['name'], n, d, str(ex))) + if t.commit(): + break def eventChannel(self, path=None): """Create an event channel to the domain. diff --git a/tools/python/xen/xend/server/DevController.py b/tools/python/xen/xend/server/DevController.py index 6c0ef2897c..0b0643c279 100644 --- a/tools/python/xen/xend/server/DevController.py +++ b/tools/python/xen/xend/server/DevController.py @@ -126,20 +126,21 @@ class DevController: compulsory to use it; subclasses may prefer to allocate IDs based upon the device configuration instead. """ - path = self.frontendMiscPath() - t = xstransact(path) - try: - result = t.read("nextDeviceID") - if result: - result = int(result) - else: - result = 1 - t.write("nextDeviceID", str(result + 1)) - t.commit() - return result - except: - t.abort() - raise + while True: + path = self.frontendMiscPath() + t = xstransact(path) + try: + result = t.read("nextDeviceID") + if result: + result = int(result) + else: + result = 1 + t.write("nextDeviceID", str(result + 1)) + if t.commit(): + return result + except: + t.abort() + raise ## private: diff --git a/tools/python/xen/xend/xenstore/xsnode.py b/tools/python/xen/xend/xenstore/xsnode.py index 817cff757a..1fb5971a78 100644 --- a/tools/python/xen/xend/xenstore/xsnode.py +++ b/tools/python/xen/xend/xenstore/xsnode.py @@ -280,8 +280,8 @@ class XenStore: (', while writing %s : %s' % (str(path), str(data)))) - def begin(self, path): - self.getxs().transaction_start(path) + def begin(self): + self.getxs().transaction_start() def commit(self, abandon=False): self.getxs().transaction_end(abort=abandon) diff --git a/tools/python/xen/xend/xenstore/xstransact.py b/tools/python/xen/xend/xenstore/xstransact.py index 170ab60b55..04e36f15f5 100644 --- a/tools/python/xen/xend/xenstore/xstransact.py +++ b/tools/python/xen/xend/xenstore/xstransact.py @@ -14,16 +14,8 @@ class xstransact: def __init__(self, path): self.in_transaction = False self.path = path.rstrip("/") - while True: - try: - xshandle().transaction_start(path) - self.in_transaction = True - return - except RuntimeError, ex: - if ex.args[0] == errno.ENOENT and path != "/": - path = "/".join(path.split("/")[0:-1]) or "/" - else: - raise + xshandle().transaction_start() + self.in_transaction = True def __del__(self): if self.in_transaction: @@ -175,14 +167,8 @@ class xstransact: t = cls(path) try: v = t.read(*args) - t.commit() - return v - except RuntimeError, ex: t.abort() - if ex.args[0] == errno.ETIMEDOUT: - pass - else: - raise + return v except: t.abort() raise @@ -194,14 +180,8 @@ class xstransact: t = cls(path) try: t.write(*args, **opts) - t.commit() - return - except RuntimeError, ex: - t.abort() - if ex.args[0] == errno.ETIMEDOUT: - pass - else: - raise + if t.commit(): + return except: t.abort() raise @@ -217,14 +197,8 @@ class xstransact: t = cls(path) try: t.remove(*args) - t.commit() - return - except RuntimeError, ex: - t.abort() - if ex.args[0] == errno.ETIMEDOUT: - pass - else: - raise + if t.commit(): + return except: t.abort() raise @@ -236,14 +210,8 @@ class xstransact: t = cls(path) try: v = t.list(*args) - t.commit() - return v - except RuntimeError, ex: - t.abort() - if ex.args[0] == errno.ETIMEDOUT: - pass - else: - raise + if t.commit(): + return v except: t.abort() raise @@ -255,14 +223,8 @@ class xstransact: t = cls(path) try: v = t.gather(*args) - t.commit() - return v - except RuntimeError, ex: - t.abort() - if ex.args[0] == errno.ETIMEDOUT: - pass - else: - raise + if t.commit(): + return v except: t.abort() raise @@ -274,14 +236,8 @@ class xstransact: t = cls(path) try: v = t.store(*args) - t.commit() - return v - except RuntimeError, ex: - t.abort() - if ex.args[0] == errno.ETIMEDOUT: - pass - else: - raise + if t.commit(): + return v except: t.abort() raise diff --git a/tools/xenstore/xenstore_client.c b/tools/xenstore/xenstore_client.c index 8b6ed4a245..ef428f9acb 100644 --- a/tools/xenstore/xenstore_client.c +++ b/tools/xenstore/xenstore_client.c @@ -14,6 +14,7 @@ #include #include #include +#include static void usage(const char *progname) @@ -82,8 +83,8 @@ main(int argc, char **argv) } #endif - /* XXX maybe find longest common prefix */ - success = xs_transaction_start(xsh, "/"); + again: + success = xs_transaction_start(xsh); if (!success) errx(1, "couldn't start transaction"); @@ -145,8 +146,10 @@ main(int argc, char **argv) out: success = xs_transaction_end(xsh, ret ? true : false); - if (!success) + if (!success) { + if (ret == 0 && errno == EAGAIN) + goto again; errx(1, "couldn't end transaction"); - + } return ret; } diff --git a/tools/xenstore/xs.h b/tools/xenstore/xs.h index 96d57cde5d..bd4bcbd348 100644 --- a/tools/xenstore/xs.h +++ b/tools/xenstore/xs.h @@ -116,8 +116,8 @@ bool xs_transaction_start(struct xs_handle *h); /* End a transaction. * If abandon is true, transaction is discarded instead of committed. - * Returns false on failure, which indicates an error: transactions will - * not fail spuriously. + * Returns false on failure: if errno == EAGAIN, you have to restart + * transaction. */ bool xs_transaction_end(struct xs_handle *h, bool abort); diff --git a/tools/xenstore/xs_tdb_dump.c b/tools/xenstore/xs_tdb_dump.c index 1cab37ed6d..a68f19fc2b 100644 --- a/tools/xenstore/xs_tdb_dump.c +++ b/tools/xenstore/xs_tdb_dump.c @@ -3,6 +3,7 @@ #include #include #include +#include #include "xs_lib.h" #include "tdb.h" -- 2.30.2